Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "inserted" metrics to processors #10372

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

djaglowski
Copy link
Member

Link to tracking issue

Resolves #10353

Testing

Added equivalent testing to other processor metrics (accepted, refused, dropped).

Documentation

Metric documentation is autogenerated.

Open Question

My initial implementation includes a breaking change to componenttest.TestTelemetry which is public facing API. If we want to avoid an immediate breaking change in this test package, I would propose the following, which I can submit in a prerequisite PR:

  1. Deprecate all TestTelemetry.Check* methods.
  2. Replace with more granular TestTelemetry.CheckOneSpecificMetric methods.

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (0d6e6bf) to head (0716a03).
Report is 12 commits behind head on main.

Current head 0716a03 differs from pull request most recent head b4bcc9f

Please upload reports for the commit b4bcc9f to get more accurate results.

Files Patch % Lines
component/componenttest/obsreporttest.go 0.00% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10372   +/-   ##
=======================================
  Coverage   92.56%   92.56%           
=======================================
  Files         387      387           
  Lines       18254    18297   +43     
=======================================
+ Hits        16896    16937   +41     
- Misses       1014     1015    +1     
- Partials      344      345    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@djaglowski djaglowski marked this pull request as ready for review June 10, 2024 15:29
@djaglowski djaglowski requested review from a team and mx-psi June 10, 2024 15:29
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with breaking API in one go, but the breaking change should appear in the API changelog.

I am not sure if there is any overlap with open-telemetry/oteps/pull/259 (I guess not in terms of the metric names, but possibly on the namespaces?). Is there a chance we would have to change these after that OTEP is finalized?

@mx-psi mx-psi requested a review from codeboten June 11, 2024 10:37
@djaglowski
Copy link
Member Author

I am okay with breaking API in one go, but the breaking change should appear in the API changelog.

Thanks, I'll add a changelog - just wanted to agree on the direction before going further with it.

I am not sure if there is any overlap with open-telemetry/oteps/pull/259 (I guess not in terms of the metric names, but possibly on the namespaces?). Is there a chance we would have to change these after that OTEP is finalized?

I think there is overlap and if the otep is accepted we will need to make quite a few changes. However, my understanding of the proposal is that all of the counts we currently have for processors, as well as these "inserted" counts would be relevant, though they would be grouped with others into new metrics. I think regardless of this PR we may end up deprecating the current metrics in favor of a new set, but both could coexist for a while.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, will wait a few days in case others want to chime in

@mx-psi mx-psi merged commit ea7270c into open-telemetry:main Jun 19, 2024
47 checks passed
@github-actions github-actions bot added this to the next release milestone Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "inserted" metrics to processorhelper
2 participants